Conversation
Automated Review URLs |
dstansby
left a comment
There was a problem hiding this comment.
Some comments, but 👍 this looks great overall
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
|
Regarding a discussion over at ome/ngff#339, I think it may be better to turn the requirement of the containing multiscales having to declare a coordinate transform to a contained label image into a SHOULD or even a MAY. Otherwise, doing the following thing essentially invalidates the parent multiscales image:
Maybe the following would be better:
|
| | Context | `input` | `output` | | ||
| |---------|---------|----------| | ||
| | **multiscales > datasets** | `{ "path": "<dataset_path>" }` | `{ "name": "intrinsic" }`| | ||
| | **multiscales > coordinateTransformations** | `{ "name": "intrinsic" }` | `{ "name": "output" }` <br> or <br> `{ "name": "intrinsic", "path": "labels/labels_path" }` | |
There was a problem hiding this comment.
I would expect any labels to be input rather than output, as the labels would be the lowest-level "leaves" of the tree that has it's apex as the "scene" at the top, with the multscale images "intrinsic" coordinateSystem in the middle.
There was a problem hiding this comment.
Can do, but that means that we'd need to weaken the requirement in the multiscales section further down, where it says:
If applications require additional transformations,
eachmultiscalesobject MAY contain the fieldcoordinateTransformations,
describing transformations that are applied to all resolution levels in the same manner.
The values of bothinputandoutputfields MUST be an object with fieldsnameandpaththat satisfy:
- The value of
inputMUST be the "intrinsic" coordinate system, referenced byname.
Thepathfield ofinputSHOULD be omitted.
The correct replacement of that statement would then be:
The value of either input or output MUST be the "intrinsic" coordinate system, referenced by name. The respective path field SHOULD be omitted.
There was a problem hiding this comment.
I think that the multiscales requirements do need to be relaxed, both to allow labels as inputs, but also because you might have intrinsic -transformed-to-> deskewed -transformed-to-> rotated then the rotation transform would not have input as intrinsic. We'd want to allow that, right?
I'm still not clear about the "intrinsic" coordinateSystem rules. Is this a term that refers to the coordinate system that behaves as the intrinsic system (all datasets output to intrinsic), or is it the case that every multiscales image MUST have a coordinateSystem that has "name": "intrinsic"?
And should viewers always attempt to show the "intrinsic" coordinateSystem? If I have e.g. the intrinsic -transformed-to-> deskewed then I may/probably want any viewer to show the deskewed coordinateSystem?
There was a problem hiding this comment.
you might have intrinsic -transformed-to-> deskewed -transformed-to-> rotated
Nope, currently not allowed. all transforms under multiscales -> coordinateTransformations must be linked to the same coordinate system (the "intrinsic" coordinate system) to limit graph complexity. So to do what you are describing you would have to choose a sequence of intrisinc -(affine + rotate)-> rotated.
About the "intrinsic" CS, I'll add a clarifying remark further up. Essentially: The "intrinsic"/"native"/"physical" coordinate system is the one that all multiscale transforms out put to.
There was a problem hiding this comment.
OK, understood. This all seems consistent now. Just unsure about "[the intrinsic coodindate system] should be used for viewing and processing unless a use case dictates otherwise".
If I'm implementing a viewer, how do I know whether to show the "intrinsic" coordinateSystem or some other (when just viewing the image, rather than the whole "scene")?
There was a problem hiding this comment.
Yeah, this definitely needs a clarifying remark further up as to what the "intrinsic" coordinate system denotes.
There was a problem hiding this comment.
Yep - I was reminded that Davis asked about that too at #118 (comment)
There was a problem hiding this comment.
Yeah, this definitely needs a clarifying remark further up as to what the "intrinsic" coordinate system denotes.
Hey :) I was thinking about that...
I'd agree with Davis' #118 (comment) that defining the intrinsic coordinate system as the "native physical coordinate system" is a bit misleading. I think the intention behind "It should be used for viewing" is important, but in my view it is already captured by the description of the transformations which have the intrinsic coordinate system as output (in the datasets objects):
In these cases, the scale transformation specifies the pixel size in physical units or time duration.
Probably that's all an implementation could know for sure about physical coordinates.
With this and the discussion in #118 (comment) in mind, how about having the definition of the intrinsic coordinate system more descriptive:
To both initialize the coordinates of a multiscale image and to define the relative scaling factors between resolution levels, multiscale images have a special coordinate system, the "intrinsic" coordinate system. It is the coordinate system that serves as the common output coordinate system for all transformations specified for the objects in the
datasetsfield of a multiscale object.
|
I think we need to review some of the existing labels statements, since we now MUST refer to labels with the input/output of a transform in the parent image.
Although the layout rules are unchanged, these statements are incomplete as the layout is not the only way to discover labels now, and labels are not only listed in the Also: "Within the multiscales object, the JSON array associated with the datasets key MUST have the same number of entries (scale levels) as the original unlabeled image". This came up at https://forum.image.sc/t/ome-zarrpari-an-ome-zarr-napari-widget/119772/9 as being overly strict. Especially if we now allow scale/translation to map labels to the parent image, this seems outdated. |
Yes, that's clearly too strict 🙈 |
|
I just thought of another issue with specifying The spec refers to a label image as "(usually having the same dimensions and coordinate transformations)" as the parent image. But it doesn't say that it MUST have the same axes. So I think we need to clarify whether label images can omit the Channel axis. If label images don't have a channel axis, then how do we handle that with a transform that goes from coordinateSystem labels (no channel) to image (with channel) |
|
If I had two arrays with different or missing dimensions I would expect some sort of broadcasting to apply: |
|
It might not be at the core of this PR, but it's not entirely clear to me from the spec text whether the "intrinsic coordinate system" needs to actually have the name "intrinsic"? Or can it be any other name and we just call this specific coordinate system the intrinsic coordinate system? In case I didn't miss it, it could make sense to clarify that. |
Fixes ome/ngff#480
Fixes ome/ngff#437
Relevant for ome/ngff#360
Description
In previous versions of the spec, we had used a mix of contentions when it came to specifying the
inputs andoutputs of coordinate transformations:scenecontext,inputandoutputhad to be an object with fieldsnameandpath.multiscales > datasets > coordinateTransformations), only a string was allowed, which had to correspond to the path of the respectivedatasetentry.multiscales > coordinateTransformations), a string was required, which had to correspond top the name of a coordinate system in the same metadata document.Following the original suggestion of @dstansby at ome/ngff#437, this is unified into a common syntax in this PR. It also has adjustments for schemas, examples and CI tests.
cc @will-moore @dstansby @clbarnes @bogovicj @lorenzocerrone @jluethi @m-albert @thewtex